Skip to content

Improve explain by predictably ordering predicates#12332

Merged
arhimondr merged 1 commit intoprestodb:masterfrom
jha-gunjan:QueryPlanChange
Feb 22, 2019
Merged

Improve explain by predictably ordering predicates#12332
arhimondr merged 1 commit intoprestodb:masterfrom
jha-gunjan:QueryPlanChange

Conversation

@jha-gunjan
Copy link
Copy Markdown

@jha-gunjan jha-gunjan commented Feb 12, 2019 •
edited
Loading

Fixes #11444 . Cherry pick of TupleDomain.java from trinodb/trino#226

Query PlanPrinter uses TupleDomain, which was giving unpredictable
order of tuples due to the use of HashMap/HashSet.
Switched to LinkedHashMap/LinkedHashSet to get predictable ordering.

@jha-gunjan jha-gunjan force-pushed the QueryPlanChange branch 2 times, most recently from 1f588e8 to fcb5c96 Compare February 13, 2019 19:56
@mbasmanova mbasmanova self-assigned this Feb 13, 2019
@jha-gunjan jha-gunjan force-pushed the QueryPlanChange branch 2 times, most recently from 42126ca to b179925 Compare February 14, 2019 00:11
@mbasmanova mbasmanova mentioned this pull request Feb 14, 2019
Closed
mbasmanova
mbasmanova reviewed Feb 14, 2019
View reviewed changes
Comment thread presto-spi/pom.xml Outdated
Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova Feb 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jha-gunjan Hmm.... SPI cannot have a dependency on guava. I guess, that means that we can't use ImmutableMap in TuppleDomain. Let's find another way.

Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova Feb 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sopel39 @findepi Karol, Piotr, would you have a suggestion on how to make TuppleDomain produce predictable order of columns?

Copy link
Copy Markdown
Contributor

@findepi findepi Feb 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbasmanova @jha-gunjan Maria, Gunjan, this is already fixed in upstream -- https://github.com/prestosql/presto
🙂

Copy link
Copy Markdown
Contributor

@findepi findepi Feb 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make something similar to trinodb/trino#226 (or simply cherry pick that).

@mbasmanova mbasmanova changed the title Changes for predictable Map iteration order using LinkedHashmap. Improve explain by predictably ordering predicates Feb 15, 2019
@mbasmanova mbasmanova requested a review from arhimondr February 15, 2019 21:52
arhimondr
arhimondr reviewed Feb 18, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't just use the ImmutableSet and ImmutableMap from Guava? These classes preserve the order.

Additionally it would be nice to have a unit test.

Comment thread presto-spi/src/main/java/com/facebook/presto/spi/predicate/TupleDomain.java
@jha-gunjan jha-gunjan force-pushed the QueryPlanChange branch 2 times, most recently from 012e7e2 to 0bd410b Compare February 21, 2019 06:52
@jha-gunjan
Copy link
Copy Markdown
Author

jha-gunjan commented Feb 21, 2019

Why don't just use the ImmutableSet and ImmutableMap from Guava? These classes preserve the order.

Additionally it would be nice to have a unit test.

Guava not imported in presto-spi.
Unit test cherry picked.

@jha-gunjan jha-gunjan closed this Feb 21, 2019
@jha-gunjan jha-gunjan reopened this Feb 21, 2019
@findepi
Copy link
Copy Markdown
Contributor

findepi commented Feb 21, 2019

Cherry pick of TupleDomain.java from prestosql/presto#226

@jha-gunjan when you're cherry picking a commit, it would be nice to retain authorship information.

@jha-gunjan
Copy link
Copy Markdown
Author

jha-gunjan commented Feb 21, 2019 •
edited
Loading

Cherry pick of TupleDomain.java from prestosql/presto#226

@jha-gunjan when you're cherry picking a commit, it would be nice to retain authorship information.

Thanks - I prefer that too - however, due to diverging branches and potential conflicts/renames - I manually cherry-picked this time. Added authorship info manually.

arhimondr
arhimondr reviewed Feb 21, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for doing this.

Few additional comments.

Also:

If you decided to address these comments, feel free to address them in a separate commit with your own authorship.

Comment thread presto-spi/src/main/java/com/facebook/presto/spi/predicate/EquatableValueSet.java Outdated
Comment thread presto-spi/src/main/java/com/facebook/presto/spi/predicate/TupleDomain.java Outdated
Comment thread presto-spi/src/main/java/com/facebook/presto/spi/predicate/TupleDomain.java Outdated
@sopel39 @jha-gunjan
Improve explain by predictably ordering query plan predicates
56c7ec0 
Fixes prestodb#11444 Improve plan stability
Uses Linked HashMap/HashSet to add ordering of keys.
@arhimondr arhimondr merged commit 5948ebd into prestodb:master Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbasmanova mbasmanova mbasmanova left review comments

@arhimondr arhimondr arhimondr approved these changes

+1 more reviewer

@findepi findepi findepi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mbasmanova mbasmanova

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jha-gunjan @findepi @arhimondr @mbasmanova @facebook-github-bot @sopel39